-
-
Notifications
You must be signed in to change notification settings - Fork 7.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Added exec template function. #1931
Conversation
tpl/template_funcs.go
Outdated
|
||
if !commandAccepted { | ||
jww.ERROR.Printf("Executing %s is not allowed. Check execWhiteList settings.", str) | ||
return "", nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Return an error instead of logging it. Easier to test, for once.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- if !commandAccepted {
execWhiteList settings.", str)jww.ERROR.Printf("Executing %s is not allowed. Check
return "", nil
Return an error instead of logging it.
Done. The test for the failure case looks a bit awkward now, let me know
if it can be improved (never did anything with Go before).
d3790d0
to
9e833ec
Compare
tpl/template_funcs_test.go
Outdated
viper.Set("ExecWhitelist", []string{}) | ||
|
||
in := "{{exec \"echo\" \"test\"}}" | ||
expectedErrSuffix := fmt.Sprintf("Executing echo is not allowed. Check execWhiteList settings.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to use fmt.Sprintf
here.
9e833ec
to
c84809f
Compare
A new template function called exec is added. This function can be used to include the standard output of an external command. Commands that are allowed to be executed must be listed within the newly introduced execWhiteList setting.
lgtm CircleCI is dumb. |
Does this pull request replace #847? |
Am 2016-03-12 12:41, schrieb digitalcraftsman:
Seems so. At least in parts. It doesn't add the short code yet (though I |
As neither of these solves the fundamental security issue, it doesn't really matter. |
In fact, I was not aware of PR #847 and the discussion when creating For instance, another possibility would be to add a new command line |
@digitalcraftsman suggested in another issue that we add a A whitelist and a --unsafe flag is the minimum here. But that is, in my eyes, still not enough. People make mistakes and suddenly the unsafe boolean is switched and the whitelist check could be bypassed by terminating the string with an emoji :-) ... |
Especially in this combination I can understand your concerns. |
@digitalcraftsman [1] suggested in another issue that we add a
savetofile template func. Combina that with exec and you will have the
perfect platform for sophisticated Trojan horses.
Actually, one of my wishes that I can directly embed some code (in some
programming language) which is compiled, executed, its output is then
embedded in the final output (so this exec is only the first step). Like
in report generation such as in knitr or pweave, but initially much
simpler.
A whitelist and a --unsafe flag is the minimum here. But that is, in
my eyes, still not enough. People make mistake ans suddenly the unsafe
boolean is switched and the whitelist check could be bypassed by
terminating the string with an emoji :-) ...
I would use hugo only as a static site generator. I agree that the
feature can be troublesome when one would use "hugo server" in
production. Is this the intended use case for hugo? Generally, I
wouldn't mind if this exec feature would simply not work for "hugo
server".
|
Unless someone has some really enlightening to add, I suggest we close this and related issues/PRs -- and focus on what we can do. A cross-platform |
Unless someone has some really enlightening to add, I suggest we close
this and related issues/PRs -- and focus on what we can do. A
cross-platform cat (file reader) would be a good start.
Of course, it is all your choice.
But what is with the suggestion to disable the feature for the "hugo
server" option? With this question I'm just trying to understand what
the exact security concerns are.
For me, hugo appears to be a static web site generator, so it converts
some input files to some output files similar to a compiler ahead of
time, i.e., there is usually a separate publishing step. If I understood
it correctly, your concern is that one could do everything with an exec
feature, which is of course right as it was the purpose of such a
command (though the feature has to be explicitly enabled and the user
has every control over it). This is essentially a feature other
programming language offers (also e.g. in LaTeX).
Also, I've just learned about the "external helpers" concept of hugo,
which means that hugo already calls external (predefined) programs.
Apparently, some flexibility is useful (and because one doesn't have
control over these tools one also cannot say that they never do
potentially bad things).
|
http://discuss.gohugo.io/ is a better place for discussions. |
49b4f8e
to
93e41a1
Compare
I'm closing this now. We may revisit this in the future. Maybe. |
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
A new template function called exec is added. This function can be used to
include the standard output of an external command. Commands that are
allowed to be executed must be listed within the newly introduced
execWhiteList setting.
This is very similar to PR #1548, especially the idea of the white list was
inspired by that request.